Skip to content

Don't allow to set up GL waveforms if GL is not available - #15762

Open
daschuer wants to merge 16 commits into
mixxxdj:2.6from
daschuer:gh14561
Open

Don't allow to set up GL waveforms if GL is not available #15762
daschuer wants to merge 16 commits into
mixxxdj:2.6from
daschuer:gh14561

Conversation

@daschuer

Copy link
Copy Markdown
Member

This fixes a crasher when no GL is available, but GL functions are used anyway. #14561

@ronso0

ronso0 commented Dec 18, 2025

Copy link
Copy Markdown
Member

Thanks for looking into this!

This PR overlaps with the changes of #15421
Do you see a chance to backport that and build the fix on top?

@daschuer

Copy link
Copy Markdown
Member Author

Yes that seems to be reasonable.

@ronso0

ronso0 commented Jan 14, 2026

Copy link
Copy Markdown
Member

Is this maybe also fixing #15858 ??
At least the stacktrace shows some GL stuff

@daschuer

Copy link
Copy Markdown
Member Author

This is now rebased on #15421 (one commit actually disappears :-) )

Is this maybe also fixing #15858 ??

I need to find time for a closer look.

@ronso0

ronso0 commented Jan 15, 2026

Copy link
Copy Markdown
Member

This is now rebased on #15421 (one commit actually disappears :-) )

Hehe..
Though there are still some old occurences of factory -> pFactory preventing the build.

@acolombier

Copy link
Copy Markdown
Member

Do we need this in 2.6?

@daschuer

Copy link
Copy Markdown
Member Author

The 2.5 branch is not affected.

@acolombier

Copy link
Copy Markdown
Member

Yes, I meant is this one considered a blocker or can it fall behind to 2.6.1?

@daschuer

Copy link
Copy Markdown
Member Author

It fixes a crash at least.

@ronso0

ronso0 commented Feb 17, 2026

Copy link
Copy Markdown
Member

@daschuer there are conflicts now. Merge 2.6 or rebase @acolombier ?

@ronso0

ronso0 commented Feb 26, 2026

Copy link
Copy Markdown
Member

The fix a6e3b02 looks good, but I can hardly give formal LGTM for a PR where the meat is my cleanup/consolidation commits from #15421

@acolombier wanna take a look?

@acolombier acolombier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is looking good, thanks both for the good cleanup! Ran a few manual tests and everything still seem to function as expected.

Let merge this in 2.6 and as this improve the GL situation with waveform,

@acolombier

Copy link
Copy Markdown
Member

@daschuer pre-commit is failing, do you want to take a look?

@daschuer

Copy link
Copy Markdown
Member Author

This is again ready for merge.

@m0dB m0dB left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@daschuer

Copy link
Copy Markdown
Member Author

@m0dB merge? Maybe someone else wants to do it.

@JoergAtGithub
JoergAtGithub requested a review from Copilot June 17, 2026 19:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Prevents crashes when OpenGL is unavailable by sanitizing backend selection and centralizing waveform option/backend handling inside WaveformWidgetFactory (issue #14561).

Changes:

  • Add factory helpers for setting/render backend acceleration and for reading/writing waveform renderer options.
  • Sanitize configured backend based on GL/GLES/shader availability.
  • Move several preference-dialog config writes/reads into WaveformWidgetFactory (e.g., beat grid alpha, waveform options).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/waveform/waveformwidgetfactory.h Exposes new APIs for acceleration and waveform option management.
src/waveform/waveformwidgetfactory.cpp Adds backend sanitization and centralizes config persistence for acceleration/options/beat grid alpha.
src/preferences/dialog/dlgprefwaveform.h Refactors private helpers to rely on factory-managed backend/option state.
src/preferences/dialog/dlgprefwaveform.cpp Uses factory APIs instead of directly manipulating config for backend/options; adjusts UI update logic accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1336 to +1361
WaveformWidgetBackend backend = m_config->getValue(
kHardwareAccelerationKey,
preferredBackend());
}
switch (backend) {
case WaveformWidgetBackend::None:
break;
case WaveformWidgetBackend::GL:
if (!m_openGlAvailable) {
backend = WaveformWidgetBackend::None;
}
break;
case WaveformWidgetBackend::GLSL:
if (!m_openGlAvailable || !m_openGLShaderAvailable) {
backend = WaveformWidgetBackend::None;
}
break;
#ifdef MIXXX_USE_QOPENGL
case WaveformWidgetBackend::AllShader:
if (!m_openGlAvailable && !m_openGlesAvailable) {
backend = WaveformWidgetBackend::None;
}
break;
#endif
}
return backend;
};
Comment on lines +700 to +713
WaveformWidgetBackend WaveformWidgetFactory::setAcceleration(bool enabled) {
WaveformWidgetBackend backend = WaveformWidgetBackend::None;
if (enabled) {
backend =
#ifdef MIXXX_USE_QOPENGL
WaveformWidgetBackend::AllShader
#else
WaveformWidgetBackend::GL
#endif
;
}
m_config->setValue(kHardwareAccelerationKey, backend);
return backend;
}
Comment on lines +1381 to +1386
allshader::WaveformRendererSignalBase::Options WaveformWidgetFactory::getWaveformOptions() {
auto options = m_config->getValue(
kWaveformOptionsKey,
allshader::WaveformRendererSignalBase::Option::None);
return options;
}
Comment thread src/waveform/waveformwidgetfactory.cpp Outdated
for (const auto& holder : std::as_const(m_waveformWidgetHolders)) {
holder.m_waveformWidget->setDisplayBeatGridAlpha(m_beatGridAlpha);
}
m_config->setValue(ConfigKey(kWaveformGroup, QStringLiteral("beatGridAlpha")), alpha);
@daschuer

Copy link
Copy Markdown
Member Author

Done

@ronso0

ronso0 commented Jul 16, 2026

Copy link
Copy Markdown
Member

clazy ant macOS now complain about congif enums 🤷‍♂️

@daschuer
daschuer force-pushed the gh14561 branch 5 times, most recently from a0083a9 to 4eb59e7 Compare July 19, 2026 12:04
@ronso0

ronso0 commented Jul 19, 2026

Copy link
Copy Markdown
Member

Now all runners stumle over that unfortunatly.

@daschuer

daschuer commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

conflicts resolved.

@ronso0

ronso0 commented Aug 5, 2026

Copy link
Copy Markdown
Member

LGTM but the configobject changes are a bit above my paygrade.
@mixxxdj/developers who can check the enum/overload commits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants